This proposal is ready for review and we welcome feedback by everyone across the whole libp2p community. Note that this proposal touches two fundamental building block of libp2p, namely addressing and protocol negotiation.

For additional context: Protocol Select builds on top of this proposal (#349).

Do we need to plan for a "phased roll out" for this, where nodes advertise both styles of multiaddr for a while?

We'll need a phased rollout, as the current code fails to properly parse / handle a security-enabled multiaddr (at least on the Go side). This is good. There are two ways to do a phased rollout here:

1. As you suggest, advertise both the old and the new, security-enabled multiaddrs at the same time.
2. Update the parsing / dialing code first, and switch to security-enabled multiaddrs once a large enough fraction of the network is able to handle these addresses.

Option 1 allows for a faster rollout, at the expense of more addresses being advertised. I think we can live with that overhead, so this would be my preference.

It seems to me that we are doing this backwards with the relay addrs.
What's wrong with /addr.../p2p-circuit/sec-proto? This allows us to easily compose and create a valid AddrInfo with /addr.../p2p-circuit/tls/p2p/QmX and has no ambiguities.

If we must introduce p2p-circuit-inner (I really want to call this p2p-circuit-sec), then it should come before the /p2p/QmX part in the multiaddr.

So the circuit multiaddr for QmX would be /relay-addr/p2p-circuit/p2p-circuit-sec/tls/p2p/QmX and not /relay-add/p2p-circuit/p2p/QmX/p2p-circuit-sec/tls.

@vyzo thank you for taking a look. I am having difficulties following your five comments above. Would you mind summarizing them in a single one?

Do I understand correctly that you would like to:

• Rename /p2p-circuit-inner to /p2p-circuit-sec?
• Move p2p-circuit-sec before the /p2p/QmX of the destination node?

3c1487a and 90bef6f change the two points discussed above. Note that instead of p2p-circuit-sec it uses p2p-circuit-security to disambiguate SECurity with SECond.

@vyzo would you mind taking another look?

Friendly ping @vyzo. Do the latest commits address your "request for change"?

If I understand correctly, we won't be able to support multiple security protocol on a single port anymore?
ie, need to do
/ip6/::1/tcp/1234/tls/
/ip6/::1/tcp/4567/noise/

That seems bad to me, it would become very difficult to, for instance, update noise to a new non-compatible version. (if that seem unlikely to you, #355 requires a new non-compatible noise version to be fixed, for instance)
The only solution would be to listen on multiple ports for the duration of the transition, which forces end-users to setup multiple ports, multiple redirection in their NAT, etc

I'm also not 100% convinced by the "downgrade" argument. Sure, it applies to mobile phones because you can't update the 3310s to safer protocols, but in our case, if a protocol is less secure, just disable it (like it was done for secio), push an update, and problem solved.
EDIT: If we want to be super safe, we could actually send the supported security protocols as part of "identify", and if we detect that the preferred one wasn't selected, kill the connection.

Finally, I'm having a hard time to imagine how active relays work in this scenario.
A: only supports tcp/noise
R: supports quic + tcp/noise
B: supports quic + noise + tls (not TCP)

How does B advertise that it handles noise? Is it supposed to advertise something like

• /ip6/::1/udp/1234/quic/
• /ip6/::1/udp/1234/quic/p2p-circuit-security/noise
• /ip6/::1/udp/1234/quic/p2p-circuit-security/tls
  ?

Weird, but ok. Except that now we're back to my first remark, when it will receive a connection coming from a relay, it will not know if it's a noise or a tls one. So again, need to listen on 2 different ports.

Now, let's be crazy and say that B also handles Webrtc, it will need to advertise:

• /ip6/::1/udp/1234/webrtc/
• /ip6/::1/udp/1234/webrtc/p2p-circuit-security/noise
• /ip6/::1/udp/1235/webrtc/p2p-circuit-security/tls
• /ip6/::1/udp/1236/quic/
• /ip6/::1/udp/1236/quic/p2p-circuit-security/noise
• /ip6/::1/udp/1237/quic/p2p-circuit-security/tls

To sum up, it seems to me that we are putting "capabilities" into the MA, and that doesn't seem right. Each MA is already a capability. My understanding is that this grew out of the goal of 0/low-RTT setups, but low RTTs can also be achieved with a negotiation.

A: supports Noise, TLS, Secio
B: supports TLS

A -> B: open connection using Noise, TLS or Secio
If you are compatible with noise, here is some early data: [the noise handshake]
B -> A: only compatible with TLS, please retry
A -> B: [TLS handshake]

You can improve on this in multiple ways (include handshake for both noise & tls in the first message, or in case of incompatibility send a TLS handshake from B to A to save half a RTT)

@Menduist Thank for you for your review.

There are multiple reasons for including the security in the multiaddr:

1. It saves one roundtrip (for a reply regarding your example see below).
2. It prevents downgrade attacks.
3. It makes it possible to disguise libp2p traffic as regular web traffic (by running /tls on port 443).

Note that is not required to run different handshake algorithms on different ports. You could also try to demux on the first (couple of) bytes of the first handshake message. Using different ports is the implementation strategy we will use in go-libp2p though.
The censorship-resistance argument is actually the convincing one for me.

I'm also not 100% convinced by the "downgrade" argument. Sure, it applies to mobile phones because you can't update the 3310s to safer protocols, but in our case, if a protocol is less secure, just disable it (like it was done for secio), push an update, and problem solved.

Sure, this is clear-cut example. If a protocol is broken, you disable it. But the more realistic option is that you (strongly) prefer one protocol over the other, even if it's not considered broken yet. This was actually the case with TLS / Noise vs. secio for the longest time.

To sum up, it seems to me that we are putting "capabilities" into the MA, and that doesn't seem right.

Another angle to look is that we're using port number to run multiple incompatible protocols on the same host, which is exactly what port numbers were invented for.

A -> B: open connection using Noise, TLS or Secio
If you are compatible with noise, here is some early data: [the noise handshake]
B -> A: only compatible with TLS, please retry
A -> B: [TLS handshake]

1. Saving a connection after an invalid message has been sent has proved quite difficult. Realistically, we'd need to introduce special framing for that.
2. You're assuming that Noise is supported by all / most implementations. That's the case today, but may not be the case in the future.

Thanks for the details, let me address the benefits you talked about:

It prevents downgrade attacks.

I would argue that since different security protocols runs on different ports, it becomes even easier to pull up a downgrade attack.
Let me introduce the Generalized MITM Libp2p Connection Downgrader™:

proc newConnectionDetected(connection):
  if connectionIsUnsafe:
    # hurray!
  else:
    blockThePort()

Since libp2p will try every address, it will eventually try a port/security protocol combo that is unsafe, or the connection won't go through. It's a bit slower than the previous attack, but can be sped up by knowing the "safe" ports before hand, and blocking them

Note that even if you manage to put every security protocol on the same port,
This attack could still work: instead of blocking the port, just block the specific message.

I presented in my first review a solution to downgrade attacks, I'll go in more detail here:
We could, once the connection is safe, restart the security negotiation. It doesn't have to block the upgrade process, and doesn't have to do anything in the end, but if we come out with a different security protocol, we kill the connection because we've been downgraded.

It's slow and stupid, but the fact that this security exist should deter anyone from trying to downgrade the connections.
It also has the added benefit that we could reuse the same system to check security protocols we don't control (eg, in quic, check that we use TLS 1.3 or whatever the case may be)
We could piggy-back by adding it to the identify / SPR, which wouldn't cause any more round trips, and would have other benefits

It saves one roundtrip

Saving a connection after an invalid message has been sent has proved quite difficult

See my proposal here: #349 (comment)
It's very straightforward, once protocol-select is implemented, I suspect this would represent ~15 LOC in nim-libp2p to add this extension

You're assuming that Noise is supported by all / most implementations

I used noise as an example, but it could be any protocol. Here are a few possibilities to improve that:

• Send supported security protocols in the SPR, this way, if you have the SPR of a peer (which will hopefully become the most common way to dial a peer), the round-trip will be saved
• If you don't have the SPR, we could add one "optimistic data" per offered protocol. It's a bit heavy, depending on the number of protocol you offer, but possible

Also, I'm may be biased because of how the nim-libp2p is used, but sometimes, the security protocols are defined by an upper spec, eg eth2

It makes it possible to disguise libp2p traffic as regular web traffic (by running /tls on port 443).

I agree. However, if we take a step back, that is, as far as I know, just a "theoretical" problem for now, ie, no one blocked libp2p using traffic pattern detection so far. If it happens one day*, we already have some transports that can't be detected (WSS, Quic, Webrtc), which would leave us enough time to apply proper countermeasures.

* I personally hope that libp2p will spread enough, and "blocking libp2p" won't just imply "I can't access eth2, ipfs and some dApps", but "I can't talk to my friends nor play this cool game". But as they say, hope for the best, plan for the worst

Other comments

Another angle to look is that we're using port number to run multiple incompatible protocols on the same host, which is exactly what port numbers were invented for.

Yes, though you don't see a new port being used everytime TLS is updated. I mean, in the 30 years HTTP existed, they only used 2 ports (80, 443), but in my last relay example I already need 4 ports.
imo, more port is just super user-unfriendly, and that applies at every level:

• libp2p users will have to setup who knows how many flags to setup every port, and transform into ma
• users of libp2p applications will have to configure theses ports in their firewall

Let's just take as an example "noise migration from /noise to /noise/2" from the perspective of a eth2 CL user:

• New release of my client
• Need to configure a new port in my .service configuration
• Need to open the port in my firewall
• [Few months later] New release of my client
• Need to notice that the old port is no longer used, and close it in my firewall

You could argue that the port should be transparent to the user, and in some cases that apply. But if you run a node worth >70k$, you probably want drastic security on it, with every unknown port blocked.
That whole upgrade process is kinda painful, and quite error-prone.

Closing thoughts

With this proposal, the downgrade is not addressed completely, the round trip could be saved in other ways.

The complexity added to relays is imo, unnecessary (if a relay is blocking libp2p.. just use another one)

imo, the only valid improvement is the ability to do /ip4/6.6.6.6/tcp/1234/tls in the case where your ISP/whatever actively blocks libp2p traffic. That's a very specific use case, that, AFAIK, no one hit yet.
However, this proposal have multiple downsides: you need more ports, security protocols upgrades becomes extra painful

So it should not become a default, but just something we could do if at some point we start to see traffic-based censorship